Skip to content

Conversation

@go-wyvern
Copy link
Member

No description provided.

Copilot AI review requested due to automatic review settings February 11, 2026 02:55
@gemini-code-assist
Copy link

Summary of Changes

Hello @go-wyvern, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the Language Server Protocol (LSP) integration by introducing a new command that allows clients to query for properties (fields and methods) of specific targets within the SPX project. This feature improves the editor's ability to provide richer contextual information, paving the way for more advanced code intelligence features like auto-completion and documentation lookups for game objects and sprites.

Highlights

  • New LSP Command: Introduced the 'xgoGetProperties' command to the Language Server Protocol (LSP) client, enabling retrieval of properties (fields and methods) for specified targets.
  • Type Definitions: Defined the 'XGoGetPropertiesParams' and 'XGoProperty' types, specifying the structure for command arguments and results, including property name, type, kind, and optional documentation.
  • Client Integration: Added a new client method, 'workspaceExecuteCommandXGoGetProperty', to facilitate the execution of the 'xgoGetProperties' command within the LSP client.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • spx-gui/src/components/editor/code-editor/lsp/index.ts
    • Imported the newly defined 'xgoGetProperties' command.
    • Added the 'workspaceExecuteCommandXGoGetProperty' method to execute the 'xgoGetProperties' command.
  • spx-gui/src/components/editor/code-editor/lsp/spxls/commands.ts
    • Defined the 'xgoGetProperties' namespace, including its command string.
    • Specified the 'XGoGetPropertiesParams' type for command arguments, requiring a 'target' string.
    • Defined the 'XGoProperty' type for command results, detailing 'name', 'type', 'kind' ('field' or 'method'), and optional 'doc'.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new SPX LSP workspace command (xgo.getProperties) to the GUI-side client/command typings so the editor can request target properties (e.g., for API reference/intellisense features).

Changes:

  • Added xgoGetProperties command definition (arguments/result typing) in spxls/commands.ts.
  • Added a corresponding SpxLSPClient wrapper method to execute the command via workspace/executeCommand.
  • Updated imports to include the new command.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
spx-gui/src/components/editor/code-editor/lsp/spxls/commands.ts Defines the new xgo.getProperties command and its argument/result types.
spx-gui/src/components/editor/code-editor/lsp/index.ts Exposes a SpxLSPClient wrapper to execute the new command and updates imports accordingly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +34 to +53
export namespace xgoGetProperties {
export const command = 'xgo.getProperties'
type XGoGetPropertiesParams = {
/** The target name, for example `Game` or a specific sprite name. */
target: string
}
export type Arguments = [XGoGetPropertiesParams]
/** A property of a target type. */
export type XGoProperty = {
/** The property name. */
name: string
/** The property type as a string. */
type: string
/** The kind of property. */
kind: 'field' | 'method'
/** Optional documentation for the property. */
doc?: string
}
export type Result = XGoProperty[]
}
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xgoGetProperties.Result is non-nullable (XGoProperty[]), but other xgo.* executeCommand results in this module are modeled as nullable (e.g., xgoRenameResources.Result, xgoGetInputSlots.Result). Since LSP workspace/executeCommand can legitimately return null, consider making this XGoProperty[] | null (and handling null at call sites) to match the established pattern and avoid potential runtime null handling bugs.

Copilot uses AI. Check for mistakes.
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new LSP command xgoGetProperties to fetch properties for a given target. The implementation looks good, but I have a couple of suggestions to improve consistency and robustness.

First, the new method in SpxLSPClient is named workspaceExecuteCommandXGoGetProperty (singular), which is inconsistent with the command itself and other similar methods. I've suggested renaming it to the plural form workspaceExecuteCommandXGoGetProperties.

Second, the result type for the new command is defined as XGoProperty[], while other similar commands can return null. To prevent potential runtime errors and maintain consistency, I've recommended changing the result type to XGoProperty[] | null.

/** Optional documentation for the property. */
doc?: string
}
export type Result = XGoProperty[]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

For consistency with other commands like xgoGetInputSlots and to handle cases where the server might not find any properties or encounters an error, it's safer to allow null as a possible result. This prevents potential runtime errors if the server returns null and the client expects an array. Please also update the return type in SpxLSPClient.workspaceExecuteCommandXGoGetProperties accordingly.

Suggested change
export type Result = XGoProperty[]
export type Result = XGoProperty[] | null

)
}

async workspaceExecuteCommandXGoGetProperties(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming Inconsistency: The method name workspaceExecuteCommandXGoGetProperty (singular) doesn't match the command namespace xgoGetProperties (plural) or the command string 'xgo.getProperties'.

This breaks the established pattern in the codebase:

  • xgoGetInputSlotsworkspaceExecuteCommandXGoGetInputSlots
  • xgoRenameResourcesworkspaceExecuteCommandXGoRenameResources
  • xgoGetPropertiesworkspaceExecuteCommandXGoGetProperty

Recommendation: Rename to workspaceExecuteCommandXGoGetProperties (plural) for consistency.

/** Optional documentation for the property. */
doc?: string
}
export type Result = XGoProperty[]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type Safety Issue: The Result type should include | null for consistency with other LSP commands.

Current: export type Result = XGoProperty[]
Compare with:

  • xgoGetInputSlots.Result = XGoInputSlot[] | null (line 31)
  • xgoRenameResources.Result = lsp.WorkspaceEdit | null (line 10)

Recommendation: Change to export type Result = XGoProperty[] | null to ensure proper type safety if the LSP server returns null.


async textDocumentDocumentLink(
ctx: RequestContext,
params: lsp.DocumentLinkParams
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Convenience Method: Consider adding a higher-level getProperties wrapper method following the established pattern in this codebase.

Existing patterns:

  • getInputSlots (line 428) wraps workspaceExecuteCommandXGoGetInputSlots
  • getCompletionItems (line 421) wraps textDocumentCompletion
  • getResourceReferences (line 389) wraps textDocumentDocumentLink

Suggested implementation:

async getProperties(ctx: RequestContext, target: string): Promise<xgoGetProperties.XGoProperty[]> {
  const result = await this.workspaceExecuteCommandXGoGetProperties(ctx, { target })
  if (result == null) return []
  return result
}

This would provide a cleaner API and improve maintainability.

export const command = 'xgo.getProperties'
type XGoGetPropertiesParams = {
/** The target name, for example `Game` or a specific sprite name. */
target: string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Input Validation: The target parameter lacks validation. Consider adding constraints to ensure it conforms to expected patterns (identifier rules or "Game" literal).

Security consideration: While the LSP runs in a sandboxed Web Worker, adding validation provides defense in depth if the WASM language server has vulnerabilities in string handling.

Recommendation:

  • Document the expected format and constraints explicitly
  • Consider runtime validation for identifiers
  • Add examples of valid vs invalid target values in the JSDoc

/** The property name. */
name: string
/** The property type as a string. */
type: string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation Enhancement: Consider adding more detailed JSDoc for the type field:

  • What format does the type string use? (Go type syntax? Simplified names?)
  • Examples of common type values (e.g., "int", "string", "func()", etc.)

This would help developers understand what to expect from the API response.

@xgopilot
Copy link
Contributor

xgopilot bot commented Feb 11, 2026

Code Review Summary

Reviewed this PR adding the xgo.getProperties LSP command. The implementation follows existing patterns well, but identified several issues requiring attention:

Critical:

  • Naming inconsistency: Method name should be plural (workspaceExecuteCommandXGoGetProperties)
  • Type safety: Result type should include | null

Recommended:

  • Add convenience wrapper method following established patterns
  • Enhance input validation and documentation
  • Consider caching for performance optimization

Overall solid implementation, just needs consistency fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant